Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip empty files in G3Reader #137

Merged
merged 2 commits into from
Jan 24, 2024
Merged

Skip empty files in G3Reader #137

merged 2 commits into from
Jan 24, 2024

Conversation

arahlin
Copy link
Member

@arahlin arahlin commented Jan 24, 2024

Avoid throwing an IO error when an empty G3 file is included in the input filenames by moving on to the next file in the list, until reaching a non-empty file or the end of the list. Issues an error message for every empty file that is encountered.

Fixes #134.

@arahlin arahlin requested a review from nwhitehorn January 24, 2024 04:04
@arahlin arahlin self-assigned this Jan 24, 2024
Avoid throwing an IO error when an empty G3 file is included in the input
filenames by silently moving on to the next file in the list until reaching a
non-empty file or the end of the list.

Fixes #134.
@nwhitehorn
Copy link
Member

Per the comment on the issue, this was a deliberate choice as a way to fail loudly on failed cluster jobs. Can we hide the new behavior behind an option so that people who turn it on are aware of consequences?

@arahlin
Copy link
Member Author

arahlin commented Jan 24, 2024

Per the comment on the issue, this was a deliberate choice as a way to fail loudly on failed cluster jobs. Can we hide the new behavior behind an option so that people who turn it on are aware of consequences?

Sure, that's probably a good idea. @samtomguns are you ok with this solution?

@arahlin
Copy link
Member Author

arahlin commented Jan 24, 2024

Per the comment on the issue, this was a deliberate choice as a way to fail loudly on failed cluster jobs. Can we hide the new behavior behind an option so that people who turn it on are aware of consequences?

Sure, that's probably a good idea. @samtomguns are you ok with this solution?

I'll add that the current failure mode is inconsistent -- if the only input file is empty, or just the first file in the list is empty, then the code runs without failure, without this fix.

@samtomguns
Copy link
Contributor

is something special about the last file from the perspective of cluster jobs? wouldn't it make more sense to have it fail on every empty file unless some extra option is turned off?

@arahlin
Copy link
Member Author

arahlin commented Jan 24, 2024

I think the right solution then is to check whether a file emitted any frames, and raise an error if it didn't (or raise a warning and skip to the next file, if the user sets that option)

@nwhitehorn
Copy link
Member

This already fails on the presence of any empty file. We can change the behavior, of course -- I just wanted everyone to be on the same page about why the current behavior exists before we do.

@nwhitehorn
Copy link
Member

Sorry, that was wrong. That was what it should have done. Since it isn't, let's just merge the PR.

Copy link
Member

@nwhitehorn nwhitehorn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per comment on issue, what this was supposed to handle was broken to begin with, so this is the closest to existing behavior as it actually is.

@arahlin
Copy link
Member Author

arahlin commented Jan 24, 2024

Sounds good. As a compromise, I've added an error message for when the reader encounters an empty file.

@arahlin arahlin merged commit df7a570 into master Jan 24, 2024
1 check passed
@arahlin arahlin deleted the skip_empty_files branch January 24, 2024 17:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

G3Reader fails on multiple files when last file is empty
3 participants